Conversation
… ts-schema-generator Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
ymc9
left a comment
There was a problem hiding this comment.
You need to run "pnpm build" from root folder. It'll trigger regeneration of schema files for zmodels used in tests. Include the updated schema files in the PR too.
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
Regenerated all test schema files with the updated import format. Commit: f7aafa7 All 22 models.ts files now use Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request updates the TypeScript schema generator and all generated model files to use explicit type-only imports ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the TypeScript schema/model generator to emit import type { ... } for ORM-only type dependencies so bundlers (e.g., Vite) don’t pull server-only ORM dependencies into client bundles.
Changes:
- Adjusted
TsSchemaGeneratorto generateimport type { ModelResult/TypeDefResult } from '@zenstackhq/orm'instead ofimport { type ... }. - Regenerated multiple checked-in
models.tsfixtures across tests to reflect the new import style. - Updated the sample generated
samples/orm/zenstack/models.tsaccordingly.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/ts-schema-generator.ts | Changes import generation to emit top-level type-only imports from @zenstackhq/orm. |
| samples/orm/zenstack/models.ts | Regenerated output to use import type for ORM type dependencies. |
| tests/runtimes/edge-runtime/schemas/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/regression/test/issue-503/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/regression/test/issue-422/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/regression/test/issue-204/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/typing/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/typed-json/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/todo/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/procedures/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/petstore/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/omit/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/name-mapping/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/json/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/delegate/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/default-auth/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/basic/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/schemas/auth-type/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/orm/plugin-infra/ext-query-args/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/github-repos/trigger.dev/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/github-repos/formbricks/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/github-repos/cal.com/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
| tests/e2e/apps/rally/zenstack/models.ts | Regenerated fixture to use import type for ORM type dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude review |
|
Claude finished @ymc9's task —— View job Code Review Complete
Review SummaryThis PR successfully addresses the core issue described in zenstackhq/zenstack#2345 by changing how TypeScript imports are generated for ORM types. The implementation is correct and comprehensive. ✅ Strengths
✅ Fix AppliedI found and fixed the inconsistency mentioned in the copilot feedback:
✅ Technical VerificationThe fix correctly addresses the original issue:
This prevents bundlers from pulling in server-side dependencies ( ✅ Code Quality
RecommendationAPPROVE - This PR correctly solves the bundling issue with a minimal, targeted change. The fix is technically sound and has been applied consistently across all generated files. |
Co-authored-by: Yiming Cao <ymc9@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot squash and merge it when all three "Build and Test" CI jobs succeed. |
I don't have the ability to squash and merge pull requests. This action needs to be performed through the GitHub UI or by someone with repository merge permissions once the CI checks pass. |
Fix: Use type-only imports for ModelResult/TypeDefResult to prevent bundling server dependencies
import { type X }as having side effects, pulling in server dependencies/home/runner/work/zenstack-v3/zenstack-v3/packages/sdk/src/ts-schema-generator.tsline 1362falsetotrueto generateimport type { ... }instead ofimport { type ... }import type { ModelResult as $ModelResult }Summary
Successfully fixed the issue by changing the TypeScript import generation from
import { type X }toimport type { X }. This prevents Vite and other bundlers from including server-side dependencies (@zenstackhq/orm, kysely, pg) in client bundles.All test schema files have been regenerated with the new import format (23 files total).
Original prompt
enums.tsfile in ZenStack CLI zenstack#2345💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit